Skip to content

Add support for podman image digests#5154

Open
mjrlee wants to merge 4 commits intoaws:mainlinefrom
mjrlee:podman
Open

Add support for podman image digests#5154
mjrlee wants to merge 4 commits intoaws:mainlinefrom
mjrlee:podman

Conversation

@mjrlee
Copy link

@mjrlee mjrlee commented Aug 3, 2023

Adds a new "engine" build argument that can be used to handle podman's different way of handing image digests.

Fixes #3170

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

@mjrlee mjrlee requested a review from a team as a code owner August 3, 2023 10:20
@mjrlee mjrlee requested review from efekarakus and removed request for a team August 3, 2023 10:20
@github-actions
Copy link

github-actions bot commented Aug 8, 2023

🍕 Here are the new binary sizes!

Name New size (kiB) size (kiB) Delta (%)
macOS (amd) 51492 51488 +0.01
macOS (arm) 51680 51684 ❤️ -0.01
linux (amd) 45324 45324 ❤️ 0.00
linux (arm) 43584 43588 ❤️ -0.01
windows (amd) 42144 42144 ❤️ 0.00

iamhopaul123
iamhopaul123 reviewed Aug 8, 2023
View reviewed changes
Copy link
Contributor

@iamhopaul123 iamhopaul123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great and thank you for contributing! Just wanted to double check: have you tested this feature locally and verified you would be able to build images using podman after this change? Or is there anything else that needs to be done for the full workflow?

internal/pkg/docker/dockerengine/dockerengine.go

// Push pushes the images with the specified tags and ecr repository URI, and returns the image digest on success.
func (c DockerCmdClient) Push(ctx context.Context, uri string, w io.Writer, tags ...string) (digest string, err error) {
func (c DockerCmdClient) Push(ctx context.Context, uri string, engine string, w io.Writer, tags ...string) (digest string, err error) {
Copy link
Contributor

@iamhopaul123 iamhopaul123 Aug 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i feel like we should have a different function for podman push (same signature as Push it's just with a different name), so that we don't need to change the function signature and also the push function is quite different when the engine type is podman.

Copy link
Author

@mjrlee mjrlee Feb 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did consider this, but it seems like there's still a lot of shared functionality that would be a pain to update in multiple places.

The alternative could be to build a new type PodmanCmdClient that implements ContainerLoginBuildPusher by just calling the existing methods - but again that seemed to add more complexity than it removed.

@bvtujo bvtujo added do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. and removed do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. labels Aug 31, 2023
@Lou1415926 Lou1415926 added area/addon Issues about addons. do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. and removed area/addon Issues about addons. do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. labels Oct 4, 2023
@Lou1415926 Lou1415926 added do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. and removed do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. labels Nov 9, 2023
@iamhopaul123 iamhopaul123 added do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. and removed do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. labels Dec 13, 2023
@iamhopaul123 iamhopaul123 added do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. and removed do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. labels Jan 17, 2024
@ammerzon
Copy link

ammerzon commented Jan 25, 2024

@mjrlee, are there any updates on this, or do you need support? 😁

@KollaAdithya KollaAdithya added do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. and removed do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. labels Jan 30, 2024
@mjrlee
Copy link
Author

mjrlee commented Feb 13, 2024

Sorry, I haven't had much time to look at this recently.

I can confirm I'm currently using it with podman and it's working well.

I've resolved a couple of comments on the PR, and replied to another.

@KollaAdithya KollaAdithya added do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. and removed do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. labels Mar 21, 2024
@huanjani huanjani added do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. and removed do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. labels Apr 18, 2024
@lucix-aws lucix-aws added do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. and removed do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. labels Apr 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@efekarakus efekarakus Awaiting requested review from efekarakus efekarakus was automatically assigned from aws/aws-ecs-bdx

@KollaAdithya KollaAdithya Awaiting requested review from KollaAdithya

1 more reviewer

@iamhopaul123 iamhopaul123 iamhopaul123 left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support Podman either as alias to Docker or natively

8 participants

@mjrlee @ammerzon @iamhopaul123 @bvtujo @huanjani @KollaAdithya @Lou1415926 @lucix-aws